Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer server service #574

Open
wants to merge 25 commits into
base: rolling
Choose a base branch
from

Conversation

tonynajjar
Copy link
Contributor

This is a rough attempt at exposing a service in the buffer server as requested in #445

Tony Najjar added 2 commits December 12, 2022 10:40
@tonynajjar
Copy link
Contributor Author

@clalancette any update here? I think this would be a significant performance boost for a lot of big projects using many TF listeners

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonynajjar I'm sorry for the extremely long time to get back to review this.

Overall, I like the concept of making this transition. I've left a bunch of fixes inline, and I also have some high-level comments:

  1. I think we should print a warning when the user uses the BufferServer action the first time. That should give them some warning that this will eventually be changing.
  2. I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

tf2_msgs/srv/LookupTransform.srv Outdated Show resolved Hide resolved
tf2_msgs/srv/LookupTransform.srv Outdated Show resolved Hide resolved
tf2_msgs/srv/LookupTransform.srv Outdated Show resolved Hide resolved
tf2_msgs/srv/LookupTransform.srv Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/buffer_server.h Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/buffer_server.h Outdated Show resolved Hide resolved
Tony Najjar and others added 12 commits November 9, 2023 13:47
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Tony Najjar <[email protected]>
@tonynajjar
Copy link
Contributor Author

I think we should print a warning when the user uses the BufferServer action the first time. That should give them some warning that this will eventually be changing.

I would simply add a RCLCPP_WARN_ONCE in the goalCB, what do you think?

I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

Can you elaborate why this is necessary? When we remove the action server in K-Turtle there won't be anything to disable, or am I misunderstanding you?

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some more fixes inline.

I think there should be a way to disable the action server. I'll suggest maybe a constructor parameter enable_lookup_action that defaults to true. When we remove the action server in K-Turtle, we can set that to false by default. I'm also open to other ideas here.

Can you elaborate why this is necessary? When we remove the action server in K-Turtle there won't be anything to disable, or am I misunderstanding you?

Regarding this question, what I was thinking about here was that creating both an action server and a service server is putting additional load on the network (particularly discovery load). For those users that know that they only want to use the new service-based interface, they could disable the action one to save themselves some discovery traffic. That said, I could be convinced that we don't need to do this.

I also realized that we should update the BufferClient to use the new service interface by default. That way most users won't notice the change at all.

tf2_ros/include/tf2_ros/buffer_server.h Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/buffer_server.h Outdated Show resolved Hide resolved
tf2_ros/include/tf2_ros/buffer_server.h Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
tf2_ros/src/buffer_server.cpp Outdated Show resolved Hide resolved
Comment on lines 188 to 189
RCLCPP_WARN_ONCE(logger_, "You are using the deprecated action interface of the tf2_ros::BufferServer. \
Please use the service interface instead.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RCLCPP_WARN_ONCE(logger_, "You are using the deprecated action interface of the tf2_ros::BufferServer. \
Please use the service interface instead.");
RCLCPP_WARN_ONCE(
logger_,
"You are using the deprecated action interface of the tf2_ros::BufferServer. "
"Please use the service interface instead.");

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Nov 13, 2023

Regarding this question, what I was thinking about here was that creating both an action server and a service server is putting additional load on the network (particularly discovery load). For those users that know that they only want to use the new service-based interface, they could disable the action one to save themselves some discovery traffic. That said, I could be convinced that we don't need to do this.

I'm still a bit confused about the need of keeping both. Here's what I propose:

In this PR, we completely replace the action interface with a service interface for both server and clients without breaking anything for most users using them "normally".

What do you think? Once this is decided I can proceed with buffer clients, tests, etc...

@clalancette
Copy link
Contributor

In this PR, we completely replace the action interface with a service interface for both server and clients without breaking anything for most users using them "normally".

It's hard to say. I don't know how many users directly connect to the action server here without using the BufferClient. If that number is ~0, then we can do this. But I generally don't like to pull the rug out from users who still might be using the old interfaces.

@tonynajjar
Copy link
Contributor Author

My view is that the intended way to use the BufferServer was together with a BufferClient so there was never a guarantee that the underlying action server will remain the same. Having an official tutorial would have helped justify this point but unfortunately I can't find any.

I can ask on the Discourse if anyone uses the action server directly if that helps. In any case, I've given my thoughts, I'd appreciate if you can come to a decision so I can implement it, as I still have only the next 2 weeks to work on contributing back this refactoring that we did internally.

@clalancette
Copy link
Contributor

I can ask on the Discourse if anyone uses the action server directly if that helps. In any case, I've given my thoughts, I'd appreciate if you can come to a decision so I can implement it, as I still have only the next 2 weeks to work on contributing back this refactoring that we did internally.

The more I look at this, the more confused I get.

I downloaded all of the releases source code for both Humble and Rolling. And within it, I can't find a single reference to BufferServer, BufferClient, or LookupTransform that isn't from this repository. That seems to suggest that not only are there no direct callers to the action, that there are no users of this code at all.

What am I missing here? Where is this code used, and how is it having extra overhead if there are no callers to it anywhere that I can find?

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Nov 20, 2023

Very interesting....... We use it extensively in our codebase to avoid each node having its own Buffer as this in itself has overhead. I guess this makes sense mostly when you have a project with many nodes needing access to transforms.
Only a speculation but I guess released packages do not have this need as they are not "example application packages". Therefore the creation of a transform listener/Buffer doesn't have much overhead. Also there is no official tutorial on how to use the buffer server/client so it won't be that popular.
One more thing, if a released package uses the buffer client, it adds a requirement for the user to launch a buffer server; it's simpler to create a TF listener and be self sufficient.

The combination of these reasons might justify why you can't find it. It doesn't mean it's not useful for projects with many tf-hungry nodes!

@gavanderhoorn
Copy link
Contributor

Just to add a 👍 to @tonynajjar's comment:

We use it extensively in our codebase to avoid each node having its own Buffer as this in itself has overhead

adding listeners 'everywhere' results in a significant amount of overhead which can be avoided by using a single buffer_server instance. Especially if a node only really needs to occasionally lookup a transform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants